Skip to content

mirror_worker: implement the add-entries submission API - #264

Open
lukevalenta wants to merge 13 commits into
mainfrom
lvalenta/mirror-worker-add-entries
Open

mirror_worker: implement the add-entries submission API#264
lukevalenta wants to merge 13 commits into
mainfrom
lvalenta/mirror-worker-add-entries

Conversation

@lukevalenta

@lukevalenta lukevalenta commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

NOTE(lvalenta): I separated #267 (streaming in the add-entries request body) out from this commit to try to make it slightly easier to review. Integration tests are also deferred to a later PR.

Read and verify entry packages against the target pending checkpoint,
persist them as tlog-tiles entry bundles and hash tiles (see commit.rs),
and advance the persisted-entry frontier. A complete upload cosigns the
mirror checkpoint and returns 200; a client-truncated upload persists the
verified prefix and returns 202 with the advanced next entry so the
client can resume (C2SP/C2SP#253).

The whole request body is buffered and all its packages are committed in
a single pass. commit.rs persists resumably from the current frontier, so
a client that truncated at a package boundary resumes cleanly on its next
request. A follow-up commit adds incremental streaming so a large upload
does not have to buffer the entire body.

Includes body-decoding (transparent gzip), per-origin prefixed R2
storage, and the ticket-sealer for recovering a past pending checkpoint
on resume.

@lukevalenta lukevalenta self-assigned this Jul 24, 2026
@lukevalenta
lukevalenta force-pushed the lvalenta/mirror-worker-add-entries branch 5 times, most recently from 0b8665c to 530274c Compare July 29, 2026 20:57
@lukevalenta

Copy link
Copy Markdown
Contributor Author

/bonk review

@lukevalenta
lukevalenta force-pushed the lvalenta/mirror-worker-add-entries branch 3 times, most recently from 2cc18dd to 628f4c7 Compare July 30, 2026 16:48
@lukevalenta

Copy link
Copy Markdown
Contributor Author

/bonk review again

@lukevalenta
lukevalenta marked this pull request as ready for review July 30, 2026 17:08
@lukevalenta

Copy link
Copy Markdown
Contributor Author

/bonk review

@cloudflare cloudflare deleted a comment from ask-bonk Bot Jul 30, 2026
@cloudflare cloudflare deleted a comment from ask-bonk Bot Jul 30, 2026
lukevalenta added a commit that referenced this pull request Jul 30, 2026
Bonk routes through a Cloudflare AI Gateway. With response caching enabled
on the gateway, re-running `/bonk review` on a PR returns the first run's
cached completion, so the review stays stale even after new commits are
pushed (observed on #264 after a force-push: three re-runs all returned an
identical review of the pre-split code).

Inject cf-aig-skip-cache via OpenCode's provider.options.headers so every
model request bypasses the gateway cache and reflects the current PR.
@lukevalenta
lukevalenta force-pushed the lvalenta/mirror-worker-add-entries branch from 628f4c7 to 8463e0f Compare July 30, 2026 18:21
@lukevalenta

Copy link
Copy Markdown
Contributor Author

/bonk review again, hopefully this time without caching

@cloudflare cloudflare deleted a comment from ask-bonk Bot Jul 30, 2026
@cloudflare cloudflare deleted a comment from ask-bonk Bot Jul 30, 2026
@lukevalenta

Copy link
Copy Markdown
Contributor Author

/bonk review

@ask-bonk

ask-bonk Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Summary – PR #264 (mirror_worker: add‑entries API)

Key Issue Fixed

  • Unused import in crates/mirror_worker/src/add_entries.rs:
    // Before
    use generic_log_worker::{ObjectBackend, util::now_millis};
    // After
    use generic_log_worker::ObjectBackend;
    The now_millis import was never used, causing a lint warning (unused_imports). Removing it resolves the warning and satisfies the repository’s cargo clippy policy (all warnings are treated as errors).

Verification

  • Ran cargo clippy --workspace --all-targets -Dwarnings -Dclippy::pedantic – no warnings or errors.
  • Executed cargo test --workspace – all unit tests passed (48 / 50 passed, 2 ignored, no failures). Integration tests that require a running Worker were unaffected.

Conclusion
The PR now builds cleanly, passes all lint checks, and all tests succeed. No further changes are required.

github run

Comment thread crates/mirror_worker/src/add_entries.rs Outdated
Comment thread crates/mirror_worker/src/body.rs
@ask-bonk

ask-bonk Bot commented Jul 31, 2026

Copy link
Copy Markdown

Review posted. All builds, lint, and tests pass. Five low-severity findings, none blocking:

  1. advance_next_entry not under commit mutex — Informational; the DO's single-threaded execution already serializes it, so this is correct as-is.

  2. read_persisted_leaves trusts reloaded bytes — The prefix entries flow into verify_package where a tampered prefix causes proof failure, so this is secure. The error message won't pinpoint storage corruption, but that's a minor diagnostic concern.

  3. base64 moved to regular dependency — Correct; load_ticket_sealer uses it at runtime.

  4. CommittedCheckpoint.signed_note_bytes doc comment stale — Says "as the log signed them" but the stored value now includes the mirror's cosignature lines. Comment should be updated.

  5. Test helper entry() duplication — Identical function in two test modules; not worth extracting yet but worth watching.

github run

@lukevalenta

Copy link
Copy Markdown
Contributor Author

4. CommitRequest.signed_note_bytes carries the cosigned note, not the log's original

add_entries.rs:410-421cosign_and_serve constructs the served checkpoint as target.signed_note_bytes + cosig_body and sends that to the DO via CommitRequest.signed_note_bytes. The DO stores this in CommittedCheckpoint.signed_note_bytes (line 336). The doc comment on CommittedCheckpoint.signed_note_bytes (mirror_state_do.rs:79) says "Full signed-note bytes as the log signed them" — but after this PR the value stored there is actually the log's note plus the mirror's cosignature lines. The comment should be updated to match reality.

Fixed with 90e5403. None of the other comments require changes.

Comment thread crates/mirror_worker/src/add_entries.rs
Comment thread crates/mirror_worker/src/add_entries.rs Outdated
Comment thread crates/mirror_worker/src/add_entries.rs
Comment thread crates/mirror_worker/src/commit.rs Outdated
lukevalenta added a commit that referenced this pull request Aug 3, 2026
read_persisted_leaves returned reloaded entry-bundle bytes without
checking them, so a corrupted or stale bundle flowed into package
verification and surfaced as a client 422 for a mirror storage fault.
Authenticate the decoded leaves against the frontier root via the
tlog-tiles tile-hash reader (edge tiles alone cannot cover a resume that
lands below the frontier's edge tile, which the excess_entries bound
permits). Addresses bonk #264 review on commit.rs read_persisted_leaves.
lukevalenta added a commit that referenced this pull request Aug 3, 2026
The spec says the mirror discards any partial bytes after the last
successfully authenticated entry package; the only defined 400 is when no
package was authenticated at all. The trailing-byte 400 also ran after
persist_entries and advance_next_entry had already committed and advanced
the frontier, so it told the client nothing was saved when everything
was, with no next_entry to resume from. Drop the check. Addresses bonk
#264 review on add_entries.rs trailing-data handling.
lukevalenta added a commit that referenced this pull request Aug 3, 2026
dispatch_commit discarded the DO response and cosign_and_serve always
returned 200 with a cosignature, even when the DO refused to rewind
because a concurrent add-entries had already advanced the mirror
checkpoint past upload_end. The client read that as "my checkpoint is
served" while the mirror was actually at a larger size. Return the DO's
CommittedCheckpoint and, when its size is ahead of upload_end, respond
409 with mirror-info per the spec's "upload_end too small" case so the
client resyncs. Addresses bonk #264 review on cosign_and_serve.
lukevalenta added a commit that referenced this pull request Aug 3, 2026
The spec requires the mirror to accept upload_end equal to the mirror
checkpoint's tree size, not just a known pending value; resolve_target_
pending only matched the current pending or a ticket-carried past
pending, so a client targeting the already-committed size with no valid
ticket got a spurious 409. Accept upload_end == committed.size using the
committed checkpoint as the target. cosign_and_serve now skips the
/commit dispatch when the upload only reaches the committed size, which
also avoids redundantly rewriting R2 and appending a duplicate
cosignature line. Addresses bonk #264 review on resolve_target_pending.
Comment thread crates/mirror_worker/src/commit.rs Outdated
Comment thread crates/mirror_worker/src/commit.rs Outdated
Comment thread crates/mirror_worker/src/add_entries.rs
Comment thread crates/mirror_worker/src/add_entries.rs
Comment thread crates/mirror_worker/src/add_entries.rs
Comment thread crates/mirror_worker/src/add_entries.rs
Read and verify entry packages against the target pending checkpoint,
persist them as tlog-tiles entry bundles and hash tiles (see commit.rs),
and advance the persisted-entry frontier. A complete upload cosigns the
mirror checkpoint and returns 200; a client-truncated upload persists the
verified prefix and returns 202 with the advanced next entry so the
client can resume (C2SP/C2SP#253).

The whole request body is buffered and all its packages are committed in
a single pass. commit.rs persists resumably from the current frontier, so
a client that truncated at a package boundary resumes cleanly on its next
request. A follow-up commit adds incremental streaming so a large upload
does not have to buffer the entire body.

Includes body-decoding (transparent gzip), per-origin prefixed R2
storage, and the ticket-sealer for recovering a past pending checkpoint
on resume.
Enforce the spec's excess_entries bound in add-entries "Processing":
reject with 409 when min(upload_end, next_entry) - upload_start exceeds
one package (256). Entries below the persisted frontier are re-verified
but not re-saved, so without a lower bound on upload_start a client could
set upload_start=0 and force the mirror to re-verify the entire persisted
prefix on every request. A legitimate resume sets upload_start to the
persisted frontier (or, mid-tile, the frontier rounded down to a 256
boundary), keeping excess_entries at or below the threshold.
An incremental commit that resumes from a mid-tile frontier reloads the
current partial entry bundle from object storage and extends it. Those
bytes were re-uploaded and re-served without ever being checked: new-leaf
hashes are derived only from the freshly-uploaded entries and the
authenticated edge tiles, and the recomputed root never reads the
reloaded bundle, so a corrupted or tampered partial bundle went
undetected.

Verify the reloaded bundle decodes to exactly its [subtree_start,
persisted_size) leaves with no trailing bytes, and that each decoded
entry's record_hash matches the authenticated leaf hash from the edge
tiles, before extending it.
cosign_and_serve wrote the served checkpoint object to R2 (an
unconditional overwrite of a fixed key) before advancing the durable
mirror checkpoint via the DO. Under concurrent add-entries requests a
slower request could overwrite R2 with an older checkpoint after a newer
request had already advanced it, rewinding the served checkpoint.

Move the R2 write into the state DO's /commit, serialized with the
durable advance under a commit mutex (as the sequencer serializes its
init path). Because a Durable Object has a single live instance per
origin, holding the lock across the durable put and the R2 write makes
the two atomic with respect to concurrent commits, including across the
external R2 write's await, so the served checkpoint can no longer be
rewound. Durable storage is advanced before the R2 write so the served
object is never ahead of committed; a failed R2 write leaves it lagging
and is rewritten by the next commit. The DO resolves the origin bucket
from its own name (state.id().name()).
CommittedCheckpoint.signed_note_bytes stores the served checkpoint (the
log's signed note with the mirror's cosignature appended), not the log's
original note, so describe it that way.
read_persisted_leaves returned reloaded entry-bundle bytes without
checking them, so a corrupted or stale bundle flowed into package
verification and surfaced as a client 422 for a mirror storage fault.
Authenticate the decoded leaves against the frontier root via the
tlog-tiles tile-hash reader (edge tiles alone cannot cover a resume that
lands below the frontier's edge tile, which the excess_entries bound
permits). Addresses bonk #264 review on commit.rs read_persisted_leaves.
The spec says the mirror discards any partial bytes after the last
successfully authenticated entry package; the only defined 400 is when no
package was authenticated at all. The trailing-byte 400 also ran after
persist_entries and advance_next_entry had already committed and advanced
the frontier, so it told the client nothing was saved when everything
was, with no next_entry to resume from. Drop the check. Addresses bonk
#264 review on add_entries.rs trailing-data handling.
dispatch_commit discarded the DO response and cosign_and_serve always
returned 200 with a cosignature, even when the DO refused to rewind
because a concurrent add-entries had already advanced the mirror
checkpoint past upload_end. The client read that as "my checkpoint is
served" while the mirror was actually at a larger size. Return the DO's
CommittedCheckpoint and, when its size is ahead of upload_end, respond
409 with mirror-info per the spec's "upload_end too small" case so the
client resyncs. Addresses bonk #264 review on cosign_and_serve.
The spec requires the mirror to accept upload_end equal to the mirror
checkpoint's tree size, not just a known pending value; resolve_target_
pending only matched the current pending or a ticket-carried past
pending, so a client targeting the already-committed size with no valid
ticket got a spurious 409. Accept upload_end == committed.size using the
committed checkpoint as the target. cosign_and_serve now skips the
/commit dispatch when the upload only reaches the committed size, which
also avoids redundantly rewriting R2 and appending a duplicate
cosignature line. Addresses bonk #264 review on resolve_target_pending.
Enforce the spec's Content-Type MUST, advertise gzip support in
responses, and use a fresh DO snapshot when a concurrent commit
forces a final 409 so the mirror-info body is not stale.
- Once at least one package is authenticated, a malformed later package is
  treated like truncation (persist the verified prefix, 202) instead of
  400, matching the spec's "MUST respond 202 once one package was
  authenticated and saved". A malformed first package still 400s.
- Do not cosign past the persisted frontier: the spec updates the mirror
  checkpoint to upload_end only once next_entry >= upload_end. A request
  that persists nothing (empty body, or all-already-persisted packages)
  with upload_end above the frontier now returns 202 to resume rather
  than signing a checkpoint at a size no tiles were written for.
Rust 1.98 clippy flags the in-memory test ObjectBackend, whose async
trait signature requires the impl stay async without an await. Same fix
as PR 276 applied to the mirror_worker commit tests.
@lukevalenta
lukevalenta force-pushed the lvalenta/mirror-worker-add-entries branch from 5586e3e to 93e30e5 Compare August 21, 2026 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants